-
Notifications
You must be signed in to change notification settings - Fork 18
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Set up graph_query testing infrastructure for task graph #325
base: master
Are you sure you want to change the base?
Conversation
Check-perf-impact results: (000e2892abb21ddd1ae413a5c86d7d95) ❓ No new benchmark data submitted. ❓ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clang-Tidy
found issue(s) with the introduced code (1/1)
fmt::format_to(std::back_inserter(dot), "{}->{}[{}];", d.node, tsk.tid, dependency_style(d.kind, d.origin)); | ||
} | ||
for(const auto& tsk : recorder.get_graph_nodes()) { | ||
const char* shape = tsk->type == task_type::epoch || tsk->type == task_type::horizon ? "ellipse" : "box style=rounded"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
variable shape
of type const char *
can be declared const
template <int Dims, typename Handler, typename Functor> | ||
void dispatch_get_access(test_utils::mock_buffer<Dims>& mb, Handler& handler, access_mode mode, Functor rmfn) { | ||
template <typename Builder, int Dims, typename Functor> | ||
auto dispatch_get_access(Builder&& builder, test_utils::mock_buffer<Dims>& mb, access_mode mode, Functor rmfn) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
forwarding reference parameter builder
is never forwarded inside the function body
} | ||
|
||
[[maybe_unused]] const task_id tid_8 = | ||
test_utils::add_host_task(tt.tm, on_master_node, [&](handler& cgh) { buf_b.get_access<access_mode::read_write>(cgh, fixed<1>({0, 128})); }); | ||
const auto tid_8 = tctx.master_node_host_task().read_write(buf_b, fixed<1>({0, 128})).submit(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Value stored to tid_8
during its initialization is never read
Pull Request Test Coverage Report for Build 12467490376Details
💛 - Coveralls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very cool! This makes me want to start the TDAG refactor.
// TODO: Can we make this the base class of cdag / idag test contexts? | ||
class tdag_test_context final : private task_manager::delegate { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How complicated would it actually be to have a common (interface) class for all four of these? We're copy pasting void device_compute()
etc a lot. I'm not too worried about this since it's just test code, but would be neat to DRY it up if it's simple enough. I remember having to repeat the same test_context
refactoring three times when I changed the fence surface API.
This brings the TDAG (mostly) in line with CDAG and IDAG. What's missing is a hierarchical structure for
task_record
, but since unlike for commands and instructions, a call tosubmit()
only creates a single task (except for horizons) it turns out we don't really need this functionality anyway.